Updated code to remove dependency on IS_LOADING_POLICY_DATA for Welcome.js #11627

techievivek · 2022-10-06T09:50:12Z

Details

In Welcome.js remove the usage of IS_LOADING_POLICY_DATA because report and policy data are loaded together so we can just rely on IS_LOADING_REPORT_DATA to trust that both report and policy data are available.

Fixed Issues

$ https://github.com/Expensify/Expensify/issues/211216
PROPOSAL: GH_LINK_ISSUE(COMMENT)

Tests

Create a new workspace from an account that's on the policyExpenseChat beta (e.g @expensifail.com) since for DEV every beta is enabled by default we can use any account.
To this newly created workspace added a member by going to Manage Members > Invite.
CMD+F and search the member's name to navigate to their workspace chat. (It could be that you will not find the workspace chat so you can just refresh the app and that should load the workspace chat).
The workspace chat would look something like this.

Write a couple of messages in the member's workspace chat, then log out of the admin account.
Validated the newly added member account and logged in to NewDot. On DEV you will use ./clitools.sh on PROD you can click on the email.
We now land in the workspace chat and we are able to see the messages sent by the admin.

Verify that no errors appear in the JS console

QA Steps

Create a new workspace from an account that's on the policyExpenseChat beta (e.g @expensifail.com).
To this newly created workspace added a member by going to Manage Members > Invite.
CMD+F and search the member's name to navigate to their workspace chat. (It could be that you will not find the workspace chat so you can just refresh the app and that should load the workspace chat).
The workspace chat would look something like this.

Write a couple of messages in the member's workspace chat, then log out of the admin account.
Validated the newly added member account and log in to NewDot.
Make sure you land in the workspace chat.

Verify that no errors appear in the JS console

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed

Screenshots

Web

Screen.Recording.2022-10-06.at.3.14.57.PM.mov

Mobile Web - Chrome

Screen.Recording.2022-10-10.at.2.47.55.PM.mov

Mobile Web - Safari

ios-safari.mov

Desktop

desktop.mov

iOS

ios.mov

Android

android.mov

luacmartins

We are intentionally removing the IS_LOADING_POLICY_DATA key. Instead of adding it back, I'd suggest that we remove the Welcome event's dependency on it.

bondydaa

Looks like the only other spots we use that Onyx key is here

App/src/libs/actions/Policy.js

Lines 183 to 203 in ec7d84b

    
           function getPolicyList() { 
        
               Onyx.set(ONYXKEYS.IS_LOADING_POLICY_DATA, true); 
        
               DeprecatedAPI.GetPolicySummaryList() 
        
                   .then((data) => { 
        
                       if (data.jsonCode !== 200) { 
        
                           Onyx.set(ONYXKEYS.IS_LOADING_POLICY_DATA, false); 
        
                           return; 
        
                       } 
        
                       const policyCollection = _.reduce(data.policySummaryList, (memo, policy) => ({ 
        
                           ...memo, 
        
                           [`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`]: getSimplifiedPolicyObject(policy, false), 
        
                       }), {}); 
        
                       if (!_.isEmpty(policyCollection)) { 
        
                           updateAllPolicies(policyCollection); 
        
                       } 
        
                       Onyx.set(ONYXKEYS.IS_LOADING_POLICY_DATA, false); 
        
                   }); 
        
           }

so we can we just fully kill this key and code?

bondydaa · 2022-10-11T22:41:29Z

src/libs/actions/App.js

+        },
+        ],


strange I thought we'd prefer what was there before (}]) did the linter complain about this?

+1 I think the other way reads better. Same for the others.

luacmartins

I agree with Bondy's comments. Let's revert the style changes in actions/App and remove this code and this Onyx key

luacmartins · 2022-10-12T14:35:10Z

src/libs/actions/App.js

+        },
+        ],


+1 I think the other way reads better. Same for the others.

bondydaa

👍 nice work.

luacmartins

Looking good! While reviewing this, I noticed that we might have another bug (not introduced in this PR)

This code is supposed to load all policies, but it actually loads all reports. So the first part of this conditional will probably always be true and admins of workspaces would also see the welcome action.

We can fix that in this PR too!

techievivek · 2022-10-13T00:06:51Z

So the first part of this conditional will probably always be true and admins of workspaces would also see the welcome action

Great catch, since policy data are not loaded this makes sense.

luacmartins

@techievivek I had to merge main to be able to see the policy expense chats. The solution seem to work well on web, desktop and mWeb Chrome, but it did not work on android, iOS or mWeb safari.

I see that you had videos with the solution working, so I'm wondering if it's just me holding something wrong. @bondydaa could you try to test this solution in iOS/android?

web.mov

desktop.mov

chrome.mov

android.mov

ios.mov

safari.mov

techievivek · 2022-10-13T15:24:29Z

That's odd, let me try testing it once again and probably adding some log statements to make sure logic is being hit.

luacmartins · 2022-10-13T15:47:12Z

In the logs, I could see that we were trying to navigate to the correct expense chat report. It might just be some RN navigation quirk.

techievivek · 2022-10-18T09:27:34Z

I struggled with the DEV environment setup yesterday, so couldn't look into it. I will test it today.

techievivek · 2022-10-18T11:35:19Z

I tested this locally and experienced the same thing that @luacmartins experienced. I wasn't able to test this further because of the Pusher limit which didn't allow me to create new accounts on DEV. But this is something that we will have to make sure gets resolved before moving forward.

In the logs, it does show that we will be navigation to the workspace chat but it just freezes.

luacmartins · 2022-10-18T16:07:00Z

@techievivek this is some quirk with react-navigation. I'd see if anybody in engineeering-chat has some ideas on how to fix this. I had a similar issue not too long ago

luacmartins · 2022-10-19T16:09:00Z

@techievivek since this is a react navigation issue, I think we should merge this to partially fix the problem and then open another issue to investigate the RN nav issue. Could you please update your branch with main? I'll review it again and merge this PR then.

techievivek · 2022-10-19T16:26:52Z

Alright, I have merged with main now.

luacmartins · 2022-10-19T16:46:03Z

@techievivek can you update your author checklist? I think that we have a new version and that's why your PR Author checklist check is failing

techievivek · 2022-10-19T19:52:29Z

@luacmartins I have ticked all the options. I am not sure why it is still failing.

techievivek · 2022-10-19T20:19:23Z

@luacmartins, thanks for the note, I have updated the checklists.

luacmartins · 2022-10-20T14:50:48Z

I have verified the author checklist is complete (all boxes are checked off).
I verified the correct issue is linked in the ### Fixed Issues section above
I verified testing steps are clear and they cover the changes made in this PR
- I verified the steps for local testing are in the Tests section
- I verified the steps for Staging and/or Production testing are in the QA steps section
- I verified the steps cover any possible failure scenarios (i.e. verify an input displays the correct error message if the entered data is not correct)
- I turned off my network connection and tested it while offline to ensure it matches the expected behavior (i.e. verify the default avatar icon is displayed if app is offline)
I checked that screenshots or videos are included for tests on all platforms
I included screenshots or videos for tests on all platforms
I verified tests pass on all platforms & I tested again on:
- iOS / native
- Android / native
- iOS / Safari
- Android / Chrome
- MacOS / Chrome
- MacOS / Desktop
If there are any errors in the console that are unrelated to this PR, I either fixed them (preferred) or linked to where I reported them in Slack
I verified proper code patterns were followed (see Reviewing the code)
- I verified that any callback methods that were added or modified are named for what the method does and never what callback they handle (i.e. toggleReport and not onIconClick).
- I verified that comments were added to code that is not self explanatory
- I verified that any new or modified comments were clear, correct English, and explained "why" the code was doing something instead of only explaining "what" the code was doing.
- I verified any copy / text shown in the product was added in all src/languages/* files
- I verified any copy / text that was added to the app is correct English and approved by marketing by adding the Waiting for Copy label for a copy review on the original GH to get the correct copy.
- I verified proper file naming conventions were followed for any new files or renamed files. All non-platform specific files are named after what they export and are not named "index.js". All platform-specific files are named for the platform the code supports as outlined in the README.
- I verified the JSDocs style guidelines (in STYLE.md) were followed
If a new code pattern is added I verified it was agreed to be used by multiple Expensify engineers
I verified that this PR follows the guidelines as stated in the Review Guidelines
I verified other components that can be impacted by these changes have been tested, and I retested again (i.e. if the PR modifies a shared library or component like Avatar, I verified the components using Avatar have been tested & I retested again)
I verified all code is DRY (the PR doesn't include any logic written more than once, with the exception of tests)
I verified any variables that can be defined as constants (ie. in CONST.js or at the top of the file that uses the constant) are defined as such
If a new component is created I verified that:
- A similar component doesn't exist in the codebase
- All props are defined accurately and each prop has a /** comment above it */
- The file is named correctly
- The component has a clear name that is non-ambiguous and the purpose of the component can be inferred from the name alone
- The only data being stored in the state is data necessary for rendering and nothing else
- For Class Components, any internal methods passed to components event handlers are bound to this properly so there are no scoping issues (i.e. for onClick={this.submit} the method this.submit should be bound to this in the constructor)
- Any internal methods bound to this are necessary to be bound (i.e. avoid this.submit = this.submit.bind(this); if this.submit is never passed to a component event handler like onClick)
- All JSX used for rendering exists in the render method
- The component has the minimum amount of code necessary for its purpose, and it is broken down into smaller components in order to separate concerns and functions
If a new CSS style is added I verified that:
- A similar style doesn't already exist
- The style can't be created with an existing StyleUtils function (i.e. StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
If the PR modifies a generic component, I tested and verified that those changes do not break usages of that component in the rest of the App (i.e. if a shared library or component like Avatar is modified, I verified that Avatar is working as expected in all cases)
If the PR modifies a component related to any of the existing Storybook stories, I tested and verified all stories for that component are still working as expected.
I have checked off every checkbox in the PR reviewer checklist, including those that don't apply to this PR.

web.mov

desktop.mov

chrome.mov

android.mov

ios.mov

safari.mov

luacmartins · 2022-10-20T14:57:56Z

@techievivek I created a new issue for the react navigation quirk on iOS and android and assigned it to you.

OSBotify · 2022-10-20T14:59:10Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-10-25T00:14:11Z

🚀 Deployed to staging by @luacmartins in version: 1.2.19-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-10-26T22:33:35Z

🚀 Deployed to production by @chiragsalian in version: 1.2.19-2 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

marcaaron · 2022-11-07T18:01:37Z

In Welcome.js remove the usage of IS_LOADING_POLICY_DATA because report and policy data are loaded together so we can just rely on IS_LOADING_REPORT_DATA to trust that both report and policy data are available.

Late to the party, but I would have re-named this if we are relaying on a "report data is loading" flag to mean more than just the report data is loading.

techievivek requested a review from luacmartins October 6, 2022 09:50

techievivek self-assigned this Oct 6, 2022

techievivek requested a review from a team as a code owner October 6, 2022 09:50

melvin-bot bot requested review from bondydaa and removed request for a team October 6, 2022 09:50

updated code to include status of loading policy data

fc1669c

techievivek force-pushed the techievivek_workspace_chat_for_first_time_user branch from b1a0ee9 to fc1669c Compare October 6, 2022 09:59

luacmartins requested changes Oct 6, 2022

View reviewed changes

techievivek added 2 commits October 10, 2022 12:13

updated code to remove loading policy data flag for Welecome.js

264f9bc

Merge branch 'main' into techievivek_workspace_chat_for_first_time_user

6406b54

techievivek requested a review from luacmartins October 10, 2022 09:33

techievivek changed the title ~~Updated code to include status of loading policy data for openApp~~ Updated code to remove dependency on IS_LOADING_POLICY_DATA for Welcome.js Oct 11, 2022

bondydaa reviewed Oct 11, 2022

View reviewed changes

luacmartins requested changes Oct 12, 2022

View reviewed changes

techievivek added 2 commits October 12, 2022 22:47

removed getPolicyList and key IS_LOADING_POLICY_DATA

6b08625

fixed linter

9aec997

techievivek requested review from bondydaa and luacmartins October 12, 2022 17:39

bondydaa previously approved these changes Oct 12, 2022

View reviewed changes

luacmartins reviewed Oct 12, 2022

View reviewed changes

fixed loading of reports in policy case

321990e

techievivek dismissed bondydaa’s stale review via 321990e October 13, 2022 00:01

techievivek requested review from luacmartins and bondydaa October 13, 2022 00:01

luacmartins reviewed Oct 13, 2022

View reviewed changes

luacmartins mentioned this pull request Oct 19, 2022

Log in - The Fab menu does not open automatically when log in with new account #10232

Closed

Merge branch 'main' into techievivek_workspace_chat_for_first_time_user

59f7091

techievivek requested a review from luacmartins October 19, 2022 16:26

luacmartins approved these changes Oct 20, 2022

View reviewed changes

luacmartins merged commit df90010 into main Oct 20, 2022

luacmartins deleted the techievivek_workspace_chat_for_first_time_user branch October 20, 2022 14:54

luacmartins mentioned this pull request Oct 20, 2022

[$1000] Fix navigating to workspace chat when a new invited user logs in #12041

Closed

OSBotify mentioned this pull request Oct 24, 2022

Deploy Checklist: New Expensify 2022-10-24 #12108

Closed

45 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updated code to remove dependency on IS_LOADING_POLICY_DATA for Welcome.js #11627

Updated code to remove dependency on IS_LOADING_POLICY_DATA for Welcome.js #11627

techievivek commented Oct 6, 2022 •

edited

Loading

luacmartins left a comment

bondydaa left a comment

bondydaa Oct 11, 2022

luacmartins Oct 12, 2022

luacmartins left a comment

luacmartins Oct 12, 2022

bondydaa left a comment

luacmartins left a comment

techievivek commented Oct 13, 2022

luacmartins left a comment •

edited

Loading

techievivek commented Oct 13, 2022

luacmartins commented Oct 13, 2022

techievivek commented Oct 18, 2022

techievivek commented Oct 18, 2022

luacmartins commented Oct 18, 2022

luacmartins commented Oct 19, 2022

techievivek commented Oct 19, 2022

luacmartins commented Oct 19, 2022

techievivek commented Oct 19, 2022

techievivek commented Oct 19, 2022

luacmartins commented Oct 20, 2022 •

edited

Loading

luacmartins commented Oct 20, 2022

OSBotify commented Oct 20, 2022

OSBotify commented Oct 25, 2022

OSBotify commented Oct 26, 2022

marcaaron commented Nov 7, 2022

	function getPolicyList() {
	Onyx.set(ONYXKEYS.IS_LOADING_POLICY_DATA, true);
	DeprecatedAPI.GetPolicySummaryList()
	.then((data) => {
	if (data.jsonCode !== 200) {
	Onyx.set(ONYXKEYS.IS_LOADING_POLICY_DATA, false);
	return;
	}

	const policyCollection = _.reduce(data.policySummaryList, (memo, policy) => ({
	...memo,
	[`${ONYXKEYS.COLLECTION.POLICY}${policy.id}`]: getSimplifiedPolicyObject(policy, false),
	}), {});

	if (!_.isEmpty(policyCollection)) {
	updateAllPolicies(policyCollection);
	}

	Onyx.set(ONYXKEYS.IS_LOADING_POLICY_DATA, false);
	});
	}

Updated code to remove dependency on IS_LOADING_POLICY_DATA for Welcome.js #11627

Updated code to remove dependency on IS_LOADING_POLICY_DATA for Welcome.js #11627

Conversation

techievivek commented Oct 6, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

PR Review Checklist

PR Author Checklist

PR Reviewer Checklist

Screenshots

Web

Mobile Web - Chrome

Mobile Web - Safari

Desktop

iOS

Android

luacmartins left a comment

Choose a reason for hiding this comment

bondydaa left a comment

Choose a reason for hiding this comment

bondydaa Oct 11, 2022

Choose a reason for hiding this comment

luacmartins Oct 12, 2022

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

luacmartins Oct 12, 2022

Choose a reason for hiding this comment

bondydaa left a comment

Choose a reason for hiding this comment

luacmartins left a comment

Choose a reason for hiding this comment

techievivek commented Oct 13, 2022

luacmartins left a comment • edited Loading

Choose a reason for hiding this comment

techievivek commented Oct 13, 2022

luacmartins commented Oct 13, 2022

techievivek commented Oct 18, 2022

techievivek commented Oct 18, 2022

luacmartins commented Oct 18, 2022

luacmartins commented Oct 19, 2022

techievivek commented Oct 19, 2022

luacmartins commented Oct 19, 2022

techievivek commented Oct 19, 2022

techievivek commented Oct 19, 2022

luacmartins commented Oct 20, 2022 • edited Loading

luacmartins commented Oct 20, 2022

OSBotify commented Oct 20, 2022

OSBotify commented Oct 25, 2022

OSBotify commented Oct 26, 2022

marcaaron commented Nov 7, 2022

techievivek commented Oct 6, 2022 •

edited

Loading

luacmartins left a comment •

edited

Loading

luacmartins commented Oct 20, 2022 •

edited

Loading